Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(notifications): Improved notification exceptions #44770

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Apr 10, 2024

Summary

Currently all methods in the notification space throw \InvalidArgumentExceptions. The problem is that there are nested calls which make it impossible to handle errors correctly or to notify wrong/invalid notifiers properly.

E.g. an app calls INotification::setLink() with an invalid value in it's INotifier::prepare(). The INotification will throw and it can go unnoticed by the INotifier as it's not meant to throw. However the IManager only sees that INotifier threw the exception and that is currently meant to be the case when the INotifier does not want to handle the INotification, so the IManager will think no one wanted to handle the notification although the respective one just had "an" error from any level. Similar problems often appear when activities are put into notifications as both providers work based on \InvalidArgumentException so none of those knows whether activity or notifications was failing actually.

This PR introduces dedicated exceptions which allow us to log them if they happened while not being expected. For the moment \InvalidArgumentException is still allowed to be thrown, and only logged on debug level, but the lib/private/Notification will only throw the new exceptions (which all extend the generic \InvalidArgumentException to keep compatibility.

  • IncompleteNotificationException - is thrown by IApp::notify when a required field was not set
  • IncompleteParsedNotificationException - is thrown by INotifier::prepare when it's IManager and a required field was not set during the rendering
  • InvalidValueException - is thrown by IAction::set* and INotification::set* when setting an invalid value (e.g. empty link, missing rich-object parameter, ...)
  • UnknownNotificationException - should be thrown by INotifier::prepare when the notification is not handled by this notifier

Companion / Sample PRs

Checklist

@nickvergessen nickvergessen added this to the Nextcloud 30 milestone Apr 10, 2024
@nickvergessen nickvergessen self-assigned this Apr 10, 2024
@nickvergessen nickvergessen marked this pull request as draft April 10, 2024 16:06
@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Apr 11, 2024
@nickvergessen nickvergessen force-pushed the techdebt/noid/improved-notification-exceptions branch from a893719 to 02c7515 Compare April 11, 2024 12:56
@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 11, 2024
@nickvergessen nickvergessen marked this pull request as ready for review April 11, 2024 13:24
lib/private/Notification/Manager.php Show resolved Hide resolved
lib/private/Notification/Manager.php Outdated Show resolved Hide resolved
lib/private/Notification/Manager.php Outdated Show resolved Hide resolved
lib/private/Notification/Manager.php Outdated Show resolved Hide resolved
lib/public/Notification/IAction.php Outdated Show resolved Hide resolved
lib/public/Notification/IDismissableNotifier.php Outdated Show resolved Hide resolved
lib/public/Notification/InvalidValueException.php Outdated Show resolved Hide resolved
… set

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the techdebt/noid/improved-notification-exceptions branch from 02c7515 to f2b6025 Compare April 12, 2024 07:24
@nickvergessen nickvergessen requested a review from come-nc April 12, 2024 07:29
… set while saving a notification

Signed-off-by: Joas Schilling <coding@schilljs.com>
…s unknown to the notifier

Signed-off-by: Joas Schilling <coding@schilljs.com>
… not parsed completely

Signed-off-by: Joas Schilling <coding@schilljs.com>
…ager when it's done

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the techdebt/noid/improved-notification-exceptions branch from f2b6025 to 8745254 Compare April 12, 2024 08:30
Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@nickvergessen nickvergessen merged commit 984f00c into master Apr 15, 2024
157 checks passed
@nickvergessen nickvergessen deleted the techdebt/noid/improved-notification-exceptions branch April 15, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants